Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix DATERANGE parser handling of clientAttrs property #5891

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Fix DATERANGE parser handling of clientAttrs property #5891

merged 1 commit into from
Oct 17, 2023

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Oct 12, 2023 •
edited
Loading

This PR will...

Replace the AttrList class clientAttrs property with a getter so that the DateRange constructor does not confuse the property with other attributes when merging DATERANGE tags with the same ID.

Why is this Pull Request needed?

Fixes logger warning DATERANGE tag attribute: "clientAttrs" does not match for tags with ID: "${DATERANGE ID}".

hls.js/src/loader/date-range.ts

Lines 41 to 50 in 470a29f

constructor(dateRangeAttr: AttrList, dateRangeWithSameId?: DateRange) {
if (dateRangeWithSameId) {
const previousAttr = dateRangeWithSameId.attr;
for (const key in previousAttr) {
if (
Object.prototype.hasOwnProperty.call(dateRangeAttr, key) &&
dateRangeAttr[key] !== previousAttr[key]
) {
logger.warn(
`DATERANGE tag attribute: "${key}" does not match for tags with ID: "${dateRangeAttr.ID}"`,

It also improves Typing since clientAttrs was treated as any but could have been string[] | undefined. Now it is always string[] as it was expected to be in the m3u8-parser:

hls.js/src/loader/m3u8-parser.ts

Lines 551 to 554 in 470a29f

substituteVariablesInAttributes(
level,
dateRangeAttr,
dateRangeAttr.clientAttrs,

Are there any points in the code the reviewer needs to double-check?

clientAttrs is only read in one place (above when parsing a new DATERANGE) so the impact of filtering attribute names in the getter should be minimal and hopefully offset by faster instantiation of AttrList instances.

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@robwalch robwalch added this to the 1.5.0 milestone Oct 12, 2023
@robwalch robwalch merged commit 664f76f into master Oct 17, 2023
@robwalch robwalch deleted the bugfix/daterange-client-attributes branch October 17, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
1 participant
@robwalch